Codecov Report

Attention: Patch coverage is 0% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 57.08%. Comparing base (5362e28) to head (2a96c8f).

❗ Current head 2a96c8f differs from pull request most recent head 2fdd7f4. Consider uploading reports for the commit 2fdd7f4 to get more accurate results

@@             Coverage Diff             @@
##           develop    #1184      +/-   ##
===========================================
+ Coverage    56.16%   57.08%   +0.91%     
===========================================
  Files          157      153       -4     
  Lines        12176    11322     -854     
===========================================
- Hits          6839     6463     -376     
+ Misses        4814     4387     -427     
+ Partials       523      472      -51     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Hi,

How does it do it?
The PR directly removes the SafeERC20.safePermit functionality from the codebase, thereby eliminating the potential vulnerability associated with it. By removing this functionality, the PR ensures that users are protected from front-running attacks when interacting with the GasSwap.sol contract.

Overall, the PR addresses a critical security concern and ensures the integrity and safety of the codebase by removing potentially exploitable functionality.

According to https://github.com/Amxx/openzeppelin-contracts/blob/487b3f9cdd0561e151d4965ca18590bba907209a/contracts/token/ERC20/extensions/IERC20Permit.sol#L16-L33, doesn't use safePermit doesn't mean users are protected from front-running attacks.

details:
In ERC20Permit.sol permit function: https://github.com/Amxx/openzeppelin-contracts/blob/487b3f9cdd0561e151d4965ca18590bba907209a/contracts/token/ERC20/extensions/ERC20Permit.sol#L44-L67

    function permit(
        address owner,
        address spender,
        uint256 value,
        uint256 deadline,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) public virtual {
        if (block.timestamp > deadline) {
            revert ERC2612ExpiredSignature(deadline);
        }

        bytes32 structHash = keccak256(abi.encode(PERMIT_TYPEHASH, owner, spender, value, _useNonce(owner), deadline));

        bytes32 hash = _hashTypedDataV4(structHash);

        address signer = ECDSA.recover(hash, v, r, s);
        if (signer != owner) {
            revert ERC2612InvalidSigner(signer, owner);
        }

        _approve(owner, spender, value);
    }

when user sign bytes32 hash, in bytes32 structHash, one of the signed data is _useNonce(owner)
so let's see what's _useNonce(address) do.

https://github.com/Amxx/openzeppelin-contracts/blob/487b3f9cdd0561e151d4965ca18590bba907209a/contracts/utils/Nonces.sol#L27-L34

function _useNonce(address owner) internal virtual returns (uint256) {
        // For each account, the nonce has an initial value of 0, can only be incremented by one, and cannot be
        // decremented or reset. This guarantees that the nonce never overflows.
        unchecked {
            // It is important to do x++ and not ++x here.
            return _nonces[owner]++;
        }
    }

so every time _useNonce(...) is triggered user nonce will increase by 1, what does this mean?
every time function permit(...) is called user nonce will increase by 1.
so here is the scenario,

1. user A signed permit hash at _nonces[A] = 0 and call function swap(...) in gasSwap.sol
2. attacker frontrun the signature and execute the function permit(...), after attacker execute permit _nonces[A] = 1
3. step 1. get executed, user signature signed at _nonces[A] = 0, but now ERC20Permit.sol expect user to sign hash at _nonces[A] = 1, so the transaction will always revert.

add contract with normal permit(no try catch) + testing here:
#1189

Also, I just pushed a fix to the test. Could you give it another look? I'm new to Hardhat. Thanks!

@Thegaram hi! any update?

hi @tapakornl we're reviewing this week, thank you very much for your patience. Update ASAP.

please move to https://github.com/scroll-tech/scroll-contracts.